Skip to content

Conversation

@epeicher
Copy link
Contributor

@epeicher epeicher commented Nov 4, 2025

Related issues

CleanShot.2025-11-05.at.18.07.23.mp4

Design RToz6tIuQ7nlZrikBte4GU-fi-10114_133424

Proposed Changes

  • Implemented new Publish site and Pull site buttons to replace the single Connect site button
  • Added some state to keep track of the different modes (Push, Pull or Connect) and the selected site when required
  • Added the SyncDialog component to the ContentTabSync
  • Added new handlers to open or not the new Dialog
  • Updated UI copy and wording throughout sync components for clarity
  • Updated modal titles and button text to reflect the operation mode
  • Added custom tooltip text support to ConnectButton component
  • Updated tests

Testing Instructions

  1. Start Studio and select a local site

  2. Navigate to the Sync tab

  3. Log in to WordPress.com if not already authenticated

  4. Test Publish Flow:

    • Click Publish site button
    • Verify the site selector modal opens with title Publish your site
    • Select a site from the list
    • Verify the sync dialog opens showing Push to [Environment]
    • Select Push and check there are no errors
  5. Test Pull Flow:

    • Click Pull site button
    • Verify the site selector modal opens with title Select a site to import
    • Select a site from the list
    • Verify the sync dialog opens showing Pull from [Environment]
    • Click on Pull and check the pull is success
  6. Test Connect Flow (when sites already exist):

    • Click Connect another site button
    • Verify the modal opens with title Connect your site
    • Select a site and verify it connects without opening the sync dialog
  7. Test Offline Mode:

    • Disconnect from the internet
    • Verify Publish site and Pull site shows tooltip with a meaningful message
    • Verify buttons are disabled when offline

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@github-actions
Copy link

github-actions bot commented Nov 4, 2025

📊 Performance Test Results

Comparing 8e46a5a vs trunk

site-editor

Metric trunk 8e46a5a Diff Change
load 10065.00 ms 11074.00 ms +1009.00 ms 🔴 10.0%

site-startup

Metric trunk 8e46a5a Diff Change
siteCreation 15183.00 ms 18117.00 ms +2934.00 ms 🔴 19.3%
siteStartup 4935.00 ms 4968.00 ms +33.00 ms 🔴 0.7%

Results are median values from multiple test runs.

Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change

@epeicher epeicher self-assigned this Nov 4, 2025
@epeicher epeicher requested a review from Copilot November 5, 2025 17:06
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the sync functionality to provide separate "Publish" and "Pull" actions instead of a generic "Connect site" button. The changes introduce a modal mode system that allows users to explicitly choose whether they want to push content to a remote site or pull content from one.

Key Changes:

  • Replaced single "Connect site" button with separate "Publish site" and "Pull site" buttons
  • Added SyncModalMode type to track the operation mode ('push' | 'pull' | 'connect')
  • Updated modal titles and button labels to reflect the selected operation mode
  • Integrated SyncDialog component to handle push/pull operations after site selection

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/modules/sync/types.ts Added SyncModalMode type definition
src/modules/sync/index.tsx Refactored to support separate push/pull workflows with modal state management
src/modules/sync/tests/index.test.tsx Updated test cases to verify publish and import button functionality
src/modules/sync/components/sync-sites-modal-selector.tsx Added mode-aware modal titles, button labels, and offline messages
src/modules/sync/components/connect-button.tsx Made tooltip text configurable via props

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@epeicher epeicher marked this pull request as ready for review November 5, 2025 17:21
@epeicher epeicher requested a review from a team November 5, 2025 17:22
Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I confirm that clicking on Publish site let me choose the site and then opens the Selective sync modal. I think works as a first iteration, but I would prefer if the modal is the "same" and it doesn't blink.
Also, I wonder if the connection should only happen after the user actually clicks on the final Push or Pull. If the user selects a remote site and then cancels the second modal, I would expect that no sites have been connected yet. That's my personal opinion, so I'm open to other opinions.

As I said, I think this is a good first iteration and I think we can merge it after 1.6.3 is out, and create a follow-up to polish the UX.

I also left some suggestions, like renaming the first modal button to Next and maybe moving the modalState to the Redux slice.

try-pull-push-main-buttons.mp4

Comment on lines 173 to 174
setModalState( ( prev ) => ( { ...prev, mode: 'push' } ) );
dispatch( connectedSitesActions.openModal() );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the modalState could be part of the Redux slice and the openModal could receive the mode as parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a great suggestion! I have actually needed to do it on #2025, I can do it in this PR though so we test with the final code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone ahead and moved to Redux, it simplifies the code a lot 38d8ae4

Comment on lines 267 to 268
dispatch( connectedSitesActions.closeModal() );
setModalState( ( prev ) => ( { ...prev, mode: null } ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the modalState would live inside the Redux slice, then we could clear the mode inside closeModal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's absolutely right, done as part of 38d8ae4

@epeicher
Copy link
Contributor Author

epeicher commented Nov 6, 2025

Thanks @sejas for your review!

Also, I wonder if the connection should only happen after the user actually clicks on the final Push or Pull. If the user selects a remote site and then cancels the second modal, I would expect that no sites have been connected yet. That's my personal opinion, so I'm open to other opinions.

That's a good point, I was hesitant to change that or not after initial implementation, but I agree it can be surprising to users if the site is connected even if they click cancel. I have implemented the change 953be86. Now, there is another blink when pulling/pushing and listing the site, but that can also be tackled as a follow-up. If we decide to go with the Connect always approach, the commit can be reverted easily.

Additionally, I have implemented all your suggestions, so I would appreciate another quick look if possible 🙏

Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epeicher , great work. Thanks addressing the feedback. Connecting the site in the last step is a huge improvement. I'll create a low priority issue to fix the blink between modals. Let's merge the PR once the V1.6.3 is out and the channel is green.
https://github.com/user-attachments/assets/5a7a1548-bfa4-4d6e-8daa-c18a44108a5b

tooltipText={ __( 'Importing a remote site requires an internet connection.' ) }
>
{ __( 'Connect site' ) }
{ __( 'Pull site' ) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have a blue border on this button according to the design:

Image

And I am currently seeing the black one:

Image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted! Changed as part of 51f1e67 in a similar way than the Export database button. Ideally, we should not need to override the style buttons, but I would tackle that as a follow-up, as it implies checking all the Buttons used to avoid regressions

__( 'Push and pull changes from your live site.' ),
__( 'Connect multiple environments.' ),
__( 'Supports staging and production sites.' ),
__( 'Sync database and file changes.' ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am finding file changes in there to sounding a bit strange, I would change it to something simpler like:

Suggested change
__( 'Sync database and file changes.' ),
__( 'Sync files and database.' ),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem changing it! It was already like that, though, and it’s in the design, so maybe worth a quick check with others? What do you think @sejas or @wojtekn?

Copy link
Contributor

@katinthehatsite katinthehatsite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, everything seems to be working as expected for me ❤️

I found an edge case where I was able to start push process for two different sites at the same time (it seems that it is not intentional). This is how I did it:

  • On a Studio site A, that does not have any sites connected, navigate to Sync tab
  • Start either Pull or Push push from the modal
  • Switch to a Studio site B that does not have any connections
  • Start another push or pull processes from the modal
  • Observe two push/ pull processes running at the same time
  • If Studio site B has sites connected already, then the Push and Pull buttons are correctly disabled

Copy link
Contributor

@katinthehatsite katinthehatsite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this is maybe a suggestion, I was thinking that it would be cool to have a Back button in the connection screen so that you can navigate back from the Sync modal to connection modal if you change your mind about the site you want to connect.

This definitely does not need to be implemented in this PR, we can just add this for consideration for @Marinatsu

@epeicher
Copy link
Contributor Author

epeicher commented Nov 7, 2025

Thanks @katinthehatsite for your review!

I found an edge case where I was able to start push process for two different sites at the same time

Well spotted! I think we should apply the same logic than to Pull and Push buttons, and disabled them when there is another site syncing, what do you think @sejas ? This would be the result:

CleanShot 2025-11-07 at 13 10 15@2x

@katinthehatsite
Copy link
Contributor

I think we should apply the same logic than to Pull and Push buttons, and disabled them when there is another site syncing, what do you think @sejas ? This would be the result:

This makes sense to me 👍

Copy link
Member

sejas commented Nov 7, 2025

Great addition! Thanks for making it consistent ✨

interface ConnectButtonProps {
variant: ButtonVariant;
connectSite?: () => void;
disableConnectButtonStyle?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was always used as true, so it has been removed, and the functionality updated

@epeicher epeicher merged commit 11271d6 into trunk Nov 7, 2025
11 checks passed
@epeicher epeicher deleted the update/connect-sites branch November 7, 2025 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants